fix: field usage when arguments have the same name as root fields#2440
Conversation
WalkthroughTwo boolean flags, Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
Comment |
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
proto/wg/cosmo/platform/v1/platform.proto (1)
2068-2069: Consider adding documentation for the new fields.The new
is_argumentandis_inputboolean fields are correctly defined and properly numbered. However, adding documentation comments would improve maintainability and help future developers understand their purpose.💡 Suggested documentation
optional string feature_flag_name = 8; + // is_argument filters field usage to only argument contexts bool is_argument = 9; + // is_input filters field usage to only input type contexts bool is_input = 10;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (6)
connect/src/wg/cosmo/platform/v1/platform_pb.tscontrolplane/src/core/bufservices/analytics/getFieldUsage.tscontrolplane/src/core/repositories/analytics/UsageRepository.tsproto/wg/cosmo/platform/v1/platform.protostudio/src/components/analytics/field-usage.tsxstudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/schema/index.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-08T20:57:07.946Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.946Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.
Applied to files:
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/schema/index.tsx
🧬 Code graph analysis (1)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/schema/index.tsx (2)
studio/src/lib/schema-helpers.ts (4)
getCategoryForType(330-374)getTypeCounts(376-404)getDeprecatedTypes(756-775)getAuthenticatedTypes(777-801)studio/src/hooks/use-workspace.ts (1)
useWorkspace(4-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: build-router
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: Analyze (go)
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: integration_test (./events)
🔇 Additional comments (8)
studio/src/components/analytics/field-usage.tsx (2)
359-361: LGTM! Clean category extraction and isInput derivation.The logic correctly derives
isInputfrom the router query parameter. Whencategoryis undefined or not "inputs",isInputdefaults tofalse, which is the correct behavior for this field usage query.
376-376: LGTM! Correct integration with the backend.The
isInputflag is properly passed to thegetFieldUsagequery, enabling the backend to filter field usage by input context as intended.connect/src/wg/cosmo/platform/v1/platform_pb.ts (2)
16035-16043: LGTM!The new boolean properties follow the correct protobuf-es generated code patterns with appropriate default values (
falsefor proto3 booleans) and proper camelCase naming convention.
16061-16062: LGTM!The field definitions are correctly structured with sequential field numbers (9, 10) that don't conflict with existing fields, and properly use
ScalarType.BOOL(T: 8) for boolean types.controlplane/src/core/bufservices/analytics/getFieldUsage.ts (1)
79-80: LGTM!Clean parameter passthrough with sensible defaults. The nullish coalescing ensures backward compatibility for callers that don't provide these new flags.
controlplane/src/core/repositories/analytics/UsageRepository.ts (2)
150-160: LGTM!The method signature correctly requires explicit boolean values for
isArgumentandisInput, which aligns with the call site providing defaults. This ensures deterministic query behavior.
182-188: No changes needed. The ClickHouse schema correctly includes theIsArgumentandIsInputcolumns (bool type) in the base table and materialized views, and the ingestion pipeline (graphqlmetrics/core/metrics_service.go) properly populates these fields. The parameterized SQL queries inUsageRepository.tsare safe and correctly target existing schema columns.studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/schema/index.tsx (1)
197-215: LGTM! TheisInputderivation is correct.The logic correctly identifies input types by checking
category === "inputs"and passes the flag to filter field usage appropriately.Note:
isArgumentis not passed here, so it defaults tofalseat the backend. This means this view will only show non-argument field usage, which appears to be the intended behavior based on the PR objective of distinguishing arguments from root fields.
…93-field-usage-confusion-for-queryholiday
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist